Summary of Changes

Hello @robertwb, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request optimizes the data serialization within the cloudpickle_pickler module by removing a redundant base64 encoding layer that was applied after compression. This change streamlines the process, allowing for direct handling of compressed data, which should lead to more efficient data handling. A potential bug has been identified where the loads function attempts to delete a non-existent variable 'c'.

Highlights

• Redundant Encoding Removed: Eliminated an unnecessary base64 encoding and decoding step from the cloudpickle_pickler module, which was previously applied after data compression.
• Serialization Optimization: Streamlined the data serialization process by directly handling compressed data, thereby reducing processing overhead and improving efficiency.
• Code Simplification: Removed the base64 import and associated encoding/decoding calls, resulting in cleaner and more concise implementations of the dumps and loads functions.
• Potential Bug Introduced: A NameError might be introduced in the loads function due to an attempt to 'del c' after the variable 'c' is no longer defined in the modified code.

Hey @robertwb , part of the effort to relax the dependency on dill is using cloudpickle to pickle deterministic types (instead of using dill)

I did not disable the compression/base64 encoding in this case because a deterministic type only has to be pickled/unpickled once per process (since the pickled type is deterministic). #36032

This is to say I think removing base64 encoding will break update compatibility, because pickled types will not be base64 encoded anymore. Can we guard this behind an update compat flag? I can make this change.

Also, do you think its worth skipping the compression step in the same change for pickling these "special" types?

Ah, so you're saying that the pickled bytes are used for coder comparison for update compatibility? That's potentially dangerous (pickles have low determinism guarantees) but I suppose works most of the time. In that case, yes, we'd likely need to guard this with a flag.

Historically dill was used to pickle the "special types" #14180, and values are pickled separately to reconstruct the instance

Not sure if you remember that PR, it has been a while, but it seems you were satisfied with the determinism of pickling special types like namedtuples, dataclasses etc.

Ah, so you're saying that the pickled bytes are used for coder comparison for update compatibility? That's potentially dangerous (pickles have low determinism guarantees) but I suppose works most of the time. In that case, yes, we'd likely need to guard this with a flag.

Thats right, whenever we change how the types are pickled, it breaks update compatibility for Shuffles in streaming pipelines, so we have to guard it by a flag.

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

OK, I'll hold off on this PR. (Looks like there's an issue with use_fake_coders not actually using fake coders that base64 was masking as well...)

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.